Skip to content

Fix CosmoTimeBasedWeatherValueFactoryTest #1470

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 12 commits into
base: dev
Choose a base branch
from
Draft

Fix CosmoTimeBasedWeatherValueFactoryTest #1470

wants to merge 12 commits into from
+70 −19

Conversation

@danielfeismann
Copy link
Member

@danielfeismann danielfeismann commented Nov 3, 2025 •
edited
Loading

@danielfeismann danielfeismann self-assigned this Nov 3, 2025
@danielfeismann danielfeismann added bug Something isn't working test Related to tests labels Nov 3, 2025
@danielfeismann danielfeismann added this to the Version 8.2 milestone Nov 3, 2025
@danielfeismann danielfeismann mentioned this pull request Nov 3, 2025
Open
@danielfeismann danielfeismann marked this pull request as ready for review November 4, 2025 14:45
@danielfeismann
Copy link
Member Author

danielfeismann commented Nov 4, 2025

To reviewer: It looks to me like when testing model == exptectedResults the test does not use the correct equals method (of TimeBasedValue) but the general Object.equals(). Maybe there is a better solution as my current workaround.

@danielfeismann
Copy link
Member Author

danielfeismann commented Nov 11, 2025

@staudtMarius: After reflecting our discussion I prefer to have two separate checks of time and values instead of model.equals(expected) to avoid the tooltip by intellij to replace it by ==...

staudtMarius
staudtMarius requested changes Nov 17, 2025
View reviewed changes
Copy link
Member

@staudtMarius staudtMarius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking care of this issue. I have some small remarks.

sebastian-peter
sebastian-peter reviewed Nov 17, 2025
View reviewed changes
Copy link
Member

@sebastian-peter sebastian-peter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danielfeismann This is very, very annoying. Apparently, groovy calls org.codehaus.groovy.runtime.typehandling.DefaultTypeTransformation#compareToWithEqualityCheck when invoking ==, and this in turn calls compareTo if the two objects implement Comparable.

A secondary problem is, that apparently we don't have consistent implementations of equals and compareTo methods (see also the documentation).

So, your current solution of comparing the fields of objects is the best we can do, imho, given Intellij's prevalence of suggesting replacements of equals with ==.

@staudtMarius
Copy link
Member

staudtMarius commented Nov 18, 2025

After a short talk with @sebastian-peter, the best way would be to use Objects.equals() to write this test.

@danielfeismann danielfeismann marked this pull request as draft November 18, 2025 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sebastian-peter sebastian-peter sebastian-peter left review comments

@staudtMarius staudtMarius Awaiting requested review from staudtMarius

Requested changes must be addressed to merge this pull request.

Assignees

@danielfeismann danielfeismann

Labels

bug Something isn't working test Related to tests

Projects

None yet

Milestone

Version 8.2

Development

Successfully merging this pull request may close these issues.

CosmoTimeBasedWeatherValueFactoryTest does not work properly

4 participants

@danielfeismann @staudtMarius @sebastian-peter